Handle missing Host: header #1579
Conversation
|
cool, just need a test for each if you dont mind! thanks man |
|
Done |
Unset host: header, change from undefined to "" Upgrade supertest to catch undefined
|
I'm not sure whether Thoughts? |
|
It should definitely be either undefined or null, since the "" (empty string) is an actual value, null is when the reference is there, but it doesn't hold anything, and undefined is when the reference doesn't exists. I think it should be null. |
|
Good idea. What about Also, what if the |
|
yeah, if it's present but empty, it should be the empty string, its the precise representation, subdomains is fine as an empty array. |
|
I'd vote undefined for host and empty array for subdomains |
Breaking change; if X-Forwarded-Host: is present but empty, it will no longer fall back to Host:
|
Done. Note that this is a breaking change; if |
| @@ -472,7 +473,10 @@ req.__defineGetter__('path', function(){ | |||
| req.__defineGetter__('host', function(){ | |||
| var trustProxy = this.app.get('trust proxy'); | |||
| var host = trustProxy && this.get('X-Forwarded-Host'); | |||
| host = host || this.get('Host'); | |||
| if (host === false || typeof host === 'undefined') // If X-Forwarded-Host is present but empty, return it. | |||
There was a problem hiding this comment.
if (!host) here should be just fine, it shouldn't ever be a boolean and then there's no need for typeof
There was a problem hiding this comment.
oh oops read that wrong, this seems a little weird to me still though
There was a problem hiding this comment.
Javascript truthiness strikes again!
This allows missing headers, or trustProxy being false.
|
Is this OK to merge? |
|
+1 please merge. A few search bots don't send the Host: header and it's irritating to have to wrap the getter with error handling. |
| @@ -445,7 +444,9 @@ req.__defineGetter__('auth', function(){ | |||
|
|
|||
| req.__defineGetter__('subdomains', function(){ | |||
| var offset = this.app.get('subdomain offset'); | |||
| return this.get('Host') | |||
| var host = this.get('Host'); | |||
There was a problem hiding this comment.
we could use this.host here since we already handle it
There was a problem hiding this comment.
Yes; in fact, subdomains currently ignores trust proxy completely, which is a bug.
Right now, a request without a
Host:header (norX-Forwarded-Host) will throw an error every time I accessreq.host.I fixed that to return an empty string.
This also affects
req.subdomains."".split(".")returns[ "" ].However, since
subdomainsslices the initial portion of the host anyway, this will still correctly return[]. (unlesssubdomain offsetis0)